-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(network): add rpc messages and codec #460
Conversation
fb701ba
to
54cf6d0
Compare
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more.
|
54cf6d0
to
c43c845
Compare
ProtocolSchema::GoodbyeV1 => "goodbye/1", | ||
ProtocolSchema::PingV1 => "ping/1", | ||
ProtocolSchema::MetadataV1 => "metadata/1", | ||
ProtocolSchema::PooledUserOpHashesV1 => "pooled_user_op_hashes/1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be an inconsistency in the spec, but the protocol id in the spec adds an "s" and calls it pooled_user_ops_hashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye, I have this on my list of changes to request to make in the spec. The protocol name should match the message name, which doesn't have the "s".
pub(crate) struct Metadata { | ||
/// Metadata sequence number | ||
pub(crate) seq_number: u64, | ||
// TODO: spec has an invalid field here, add if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this referring to mempool_nets
? Why is it invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another thing I wanted to discuss with the spec writers, I don't believe the mempool nets field is valid as defined. There currently is no definition for a "mempool id gossip subnets". We use the gossip topic to signify different domains for mempool ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good from what I can see!
|
||
#[test] | ||
fn test_encode_decode() { | ||
let mut codec = LengthPrefixedSnappyCodec::new(0, 1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reuse of the same codec definition
ProtocolSchema::StatusV1 => "status/1", | ||
ProtocolSchema::GoodbyeV1 => "goodbye/1", | ||
ProtocolSchema::PingV1 => "ping/1", | ||
ProtocolSchema::MetadataV1 => "metadata/1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the /1
endpoints are in the spec but I hate it haha. I like the /v1/method
endpoint style. Just a rant, don't mind me
Starts #248
Proposed Changes
rundler-network
crateNotes:
Next up: